Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path validation: builder/verifier API skeletons #9405

Merged
merged 72 commits into from
Sep 16, 2023

Conversation

woodruffw
Copy link
Contributor

This breaks out #8873, including everything except the top-level verify APIs (both Python and Rust).

Signed-off-by: William Woodruff <[email protected]>

validation: remove Profile abstract from public APIs

One step towards removing it entirely

Signed-off-by: William Woodruff <[email protected]>

policy: disambiguate references

Signed-off-by: William Woodruff <[email protected]>

policy: remove separate rfc5280 profile

Signed-off-by: William Woodruff <[email protected]>

policy: remove profile abstraction entirely

Signed-off-by: William Woodruff <[email protected]>

rust: permitted_algorithms filtering

Signed-off-by: William Woodruff <[email protected]>

verify: simplify policy API substantially

No more manual monomorphization.

Signed-off-by: William Woodruff <[email protected]>

src, tests: remove verification code

Signed-off-by: William Woodruff <[email protected]>

validation: remove more validation code

Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw force-pushed the tob-x509-api-breakout branch from 0b085cd to 36fef28 Compare August 11, 2023 19:19
@alex
Copy link
Member

alex commented Aug 14, 2023

I think we're going to want to break this down even further: basically take each type in the public API, and put that into its own PR.

@woodruffw woodruffw mentioned this pull request Aug 14, 2023
@woodruffw
Copy link
Contributor Author

I think we're going to want to break this down even further: basically take each type in the public API, and put that into its own PR.

Opened #9411 for just the Store bits; I'll do the same for others.

woodruffw added a commit to trail-of-forks/cryptography-old that referenced this pull request Aug 14, 2023
Breakout from pyca#9405.

Signed-off-by: William Woodruff <[email protected]>
alex pushed a commit that referenced this pull request Aug 14, 2023
@woodruffw
Copy link
Contributor Author

Just to keep things synchronized here: this is blocked on #9548; once that's merged, we can deconflict here and determine what (if anything else) needs to be split out to get this to a reviewable size.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Not used, yet.

Signed-off-by: William Woodruff <[email protected]>
This reverts commit a5154e1.
Paring down for review.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
For now.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw changed the title Path validation: builder and verifier APIs Path validation: builder/verifier API skeletons Sep 15, 2023
src/rust/cryptography-x509-validation/Cargo.toml Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/policy/mod.rs Outdated Show resolved Hide resolved
src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
src/rust/src/x509/verify.rs Show resolved Hide resolved
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Since it doesn't actually do anything WebPKI related at the moment.

Signed-off-by: William Woodruff <[email protected]>
@@ -201,7 +201,6 @@ pub static UNRECOGNIZED_EXTENSION: LazyPyImport =
LazyPyImport::new("cryptography.x509", &["UnrecognizedExtension"]);
pub static EXTENSION: LazyPyImport = LazyPyImport::new("cryptography.x509", &["Extension"]);
pub static EXTENSIONS: LazyPyImport = LazyPyImport::new("cryptography.x509", &["Extensions"]);
pub static IPADDRESS: LazyPyImport = LazyPyImport::new("cryptography.x509", &["IPAddress"]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: I moved this down and renamed it to make it consistent with DNS_NAME + co-locate it with the other exported name types (DNS_NAME and RFC822_NAME).


The verifier's validation time. If not specified during construction
(see :class:`PolicyBuilder`), this is set to
:meth:`datetime.datetime.now`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if not specified should go in the policy builder docs, and should state that it's now at the time you call build

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -43,6 +47,125 @@ impl CryptoOps for PyCryptoOps {
}
}

struct FixedPolicy<'a>(Policy<'a, PyCryptoOps>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better name than FixedPolicy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe BoundPolicy (to emphasize that we're binding a Policy to its PyCryptoOps? Or maybe I could just import Policy as _Policy or similar and rename this to Policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about PyCryptoPolicy :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +54 to +58
// TODO: Switch this to `Py<PyString>` once Pyo3's `to_str()` preserves a
// lifetime relationship between an a `PyString` and its borrowed `&str`
// reference in all limited API builds. PyO3 can't currently do that in
// older limited API builds because it needs `PyUnicode_AsUTF8AndSize` to do
// so, which was only stabilized with 3.10.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to block this, but would be good to send a PR that implements the method for Python >= 3.10.

Comment on lines 59 to 60
DNSName((pyo3::Py<pyo3::PyAny>, String)),
IPAddress((pyo3::Py<pyo3::PyAny>, pyo3::Py<pyo3::types::PyBytes>)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be marginally cleaner if there was a seperate py_subject field on the Verifier, and then SubjectOwner's enum elements only needed to be a single field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
Signed-off-by: William Woodruff <[email protected]>
Comment on lines 71 to 75
// TODO: Switch this to `Py<PyString>` once Pyo3's `to_str()` preserves a
// lifetime relationship between an a `PyString` and its borrowed `&str`
// reference in all limited API builds. PyO3 can't currently do that in
// older limited API builds because it needs `PyUnicode_AsUTF8AndSize` to do
// so, which was only stabilized with 3.10.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this goes in SubjectOwner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, bad copy. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// reference in all limited API builds. PyO3 can't currently do that in
// older limited API builds because it needs `PyUnicode_AsUTF8AndSize` to do
// so, which was only stabilized with 3.10.
py_subject: pyo3::Py<pyo3::PyAny>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use pyo3(get, name = "subject") here (https://pyo3.rs/v0.19.2/class#object-properties-using-pyo3get-set)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@alex alex enabled auto-merge (squash) September 16, 2023 20:47
@alex alex merged commit 73d070e into pyca:main Sep 16, 2023
58 checks passed
@woodruffw woodruffw deleted the tob-x509-api-breakout branch September 16, 2023 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants